Rename setPaymentDisabled to setPaymentStatus #474
Conversation
Also changes constant to view app-wide
0xKiwi
changed the title
apps/finance/contracts/Finance.sol
Outdated
| */ | ||
| function setPaymentDisabled(uint256 _paymentId, bool _disabled) | ||
| function setPaymentStatus(uint256 _paymentId, bool _active) | ||
| external | ||
| authP(DISABLE_PAYMENTS_ROLE, arr(_paymentId)) |
There was a problem hiding this comment.
We should also rename this role, so it's less confusing. It can actually enable or disable existing payments, so I'd rename it to MANAGE_PAYMENTS_ROLE.
Although @izqui we should probably think of a way to completely disable a payment at some point (or remove the ability to re-enable payments).
| @@ -192,7 +192,7 @@ contract MiniMeToken is Controlled { | |||
| require((_to != 0) && (_to != address(this))); | |||
| // If the amount being transfered is more than the balance of the | |||
| // account the transfer returns false | |||
| var previousBalanceFrom = balanceOfAt(_from, block.number); | |||
| uint256 previousBalanceFrom = balanceOfAt(_from, block.number); | |||
There was a problem hiding this comment.
Could you revert the changes to the MiniMeToken here?
I'm not opposed to fixing the compiler warnings here, but we'd also like it to stay as closely as possible to the original version and we should have a separate discussion if we should just update to MMT_0.2.
|
Just to re-surface this from #474 (comment):
|
|
Should be good to go now! 😄 |
|
What is this PR waiting on? |
|
Just a final review, @IvanTheGreatDev! Just added 51539ca passing whether we are setting the payment as active as a parameter to the role. Thanks for the contribution :) |
* Renames setPaymentDisabled to setPaymentStatus Also changes constant to view app-wide * Rename bytes32 and add extra test for status * Revert MiniMe to previous state * Add active boolean as a param to MANAGE_PAYMENTS_ROLE
Also changes constant to view app-wide and renamed app-wide from disabled to inactive for the payment status.
#374